Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce perf spec #478

Merged
merged 7 commits into from
May 31, 2023
Merged

Introduce perf spec #478

merged 7 commits into from
May 31, 2023

Conversation

MarcoPolo
Copy link
Contributor

A simple protocol to run client-driven benchmarks inspired by https://datatracker.ietf.org/doc/html/draft-banks-quic-performance.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposal looks good to me. Thanks for the write-up!

Is there any value in merging this early? If not, I would suggest holding off until we have a first implementation.

perf/perf.md Show resolved Hide resolved
perf/perf.md Show resolved Hide resolved
perf/perf.md Outdated Show resolved Hide resolved
perf/perf.md Show resolved Hide resolved
perf/perf.md Show resolved Hide resolved
perf/perf.md Outdated
Comment on lines 53 to 54
2. Read from the stream until we get an `EOF` (client's write side was closed).
3. Send the number of bytes defined in step 1 back to the client.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would there be a benefit of sending things back at the same time at least as an option rather than waiting til the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would opening two streams in parallel be enough to satisfy this use case?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe full-duplex throughput within a single stream is interesting as well? This could be an optional flag in the client's request.

The referenced draft does ask the question whether it should be possible to send the traffic from the server before FIN:

Note - Should the server wait for FIN before replying?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can get most of the value with using two streams. The full-duplex is interesting, but I'd hold off from doing that until we have a compelling use case since it adds a bit of complexity to support that option.

@mxinden
Copy link
Member

mxinden commented Feb 26, 2023

Wrote a draft implementation of this specification on top of rust-libp2p. See libp2p/rust-libp2p#3508.

Draft only for now. Don't (yet) expect this to produce reliable performance metrics.

Server implementation is deployed at /dns4/libp2p-perf.max-inden.de/tcp/4001.

Easiest way to test from your machine:

docker run -ti --rm --entrypoint perf-client mxinden/libp2p-perf --server-address /dns4/libp2p-perf.max-inden.de/tcp/4001

Finished run: Sent 10 MiB in 9.497557834 s with 8.423217989114063 MiBit/s and received 10 MiB in 6.494584597 s with 12.317954875351667 MiBit/s

For server machine metrics, see https://libp2p-perf.max-inden.de/d/rYdddlPWk/node-exporter-full

mergify bot pushed a commit to libp2p/rust-libp2p that referenced this pull request Mar 19, 2023
Implementation of the libp2p perf protocol according to libp2p/specs#478.

//CC @MarcoPolo  as the author of the specification.

**Don't (yet) expect this to produce reliable performance metrics.**

Pull-Request: #3508.
@mxinden
Copy link
Member

mxinden commented Mar 19, 2023

Is there any value in merging this early? If not, I would suggest holding off until we have a first implementation.

libp2p/rust-libp2p#3508 just merged in case you were holding off merging here till there is at least one implementation @MarcoPolo.

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of stylistic notes. Take however you wish :)


# Protocol

The `/perf/1.0.0` protocol (from here on referred to as simply `perf`) is a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we wanted to stop the use of numbers in the protocols in their first version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this documented somewhere? I don't particularly care one way or the other, but we already have a couple implementations out and this would be a bit annoying of a change with little to gain so I would opt for not making the change.

perf/perf.md Show resolved Hide resolved
perf/perf.md Show resolved Hide resolved
perf/perf.md Outdated Show resolved Hide resolved
perf/perf.md Show resolved Hide resolved
perf/perf.md Show resolved Hide resolved
perf/perf.md Outdated Show resolved Hide resolved
perf/perf.md Outdated Show resolved Hide resolved
- [Security Considerations](#security-considerations)
- [Prior Art](#prior-art)

# Context
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Context
## Context

Nit: Multiple L1 headings in this document.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is that a problem? This is a top level section. If we made this ## then it would be a child of ToC, which is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see the argument now. This could all be a child of the # Perf part. I think that's a good argument, but the counter argument is that then you would always lose one nesting level in this style since you'll always have that title heading.

Unless we have some specific templating system we are trying to make happy, I don't think it's necessary to only have a single # level. This whole doc is about perf, so it should be okay to have multiple # levels.

perf/perf.md Outdated Show resolved Hide resolved
MarcoPolo and others added 4 commits May 31, 2023 09:47
@MarcoPolo
Copy link
Contributor Author

Merging this since we have 4 implementations out already (Rust, Go, JS, and Zig). I'll leave it as 1A for now since all of these are prototypes and we haven't merged anything yet (with the exception of the Rust impl). We can update that in a future PR.

@MarcoPolo MarcoPolo merged commit 917be26 into master May 31, 2023
@MarcoPolo MarcoPolo deleted the marco/perf branch May 31, 2023 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.